Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggest using type args directly instead of equality constraint #122591

Merged
merged 1 commit into from
Apr 23, 2024

Conversation

gurry
Copy link
Contributor

@gurry gurry commented Mar 16, 2024

When type arguments are written erroneously using an equality constraint we suggest specifying them directly without the equality constraint.

Fixes #122162

Changes the diagnostic in the issue from:

error[E0229]: associated type bindings are not allowed here
9 | impl std::cmp::PartialEq<Rhs = T> for S {
  |                          ^^^^^^^ associated type not allowed here
  |

to

error[E0229]: associated type bindings are not allowed here
9 | impl std::cmp::PartialEq<Rhs = T> for S {
  |                          ^^^^^^^ associated type not allowed here
  |
help: to use `T` as a generic argument specify it directly
  |
  |      impl std::cmp::PartialEq<T> for S {
  |                               ~

@rustbot
Copy link
Collaborator

rustbot commented Mar 16, 2024

r? @lcnr

rustbot has assigned @lcnr.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 16, 2024
@bors
Copy link
Contributor

bors commented Mar 21, 2024

☔ The latest upstream changes (presumably #121123) made this pull request unmergeable. Please resolve the merge conflicts.

@gurry
Copy link
Contributor Author

gurry commented Mar 22, 2024

@rustbot label -S-waiting-on-review +S-waiting-on-author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 22, 2024
@lcnr
Copy link
Contributor

lcnr commented Mar 22, 2024

r? @fmease

@rustbot rustbot assigned fmease and unassigned lcnr Mar 22, 2024
@gurry gurry force-pushed the 122162-impl-type-binding-suggestion branch from d29a5a1 to 560d463 Compare March 22, 2024 11:48
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Mar 22, 2024

☔ The latest upstream changes (presumably #120926) made this pull request unmergeable. Please resolve the merge conflicts.

@gurry gurry force-pushed the 122162-impl-type-binding-suggestion branch 2 times, most recently from 3aec774 to ec93aaa Compare March 22, 2024 14:20
@rustbot
Copy link
Collaborator

rustbot commented Mar 22, 2024

HIR ty lowering was modified

cc @fmease

@rust-log-analyzer

This comment has been minimized.

@gurry gurry force-pushed the 122162-impl-type-binding-suggestion branch from ec93aaa to 274ad2a Compare March 23, 2024 03:16
@gurry
Copy link
Contributor Author

gurry commented Mar 23, 2024

@rustbot label +S-waiting-on-review -S-waiting-on-author

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 23, 2024
@gurry gurry force-pushed the 122162-impl-type-binding-suggestion branch from 274ad2a to f31eb72 Compare March 23, 2024 08:31
compiler/rustc_hir_analysis/src/hir_ty_lowering/errors.rs Outdated Show resolved Hide resolved
tests/rustdoc-ui/invalid_associated_const.stderr Outdated Show resolved Hide resolved
compiler/rustc_hir_analysis/src/hir_ty_lowering/errors.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_analysis/src/hir_ty_lowering/errors.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_analysis/src/hir_ty_lowering/errors.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_analysis/src/hir_ty_lowering/errors.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_analysis/src/hir_ty_lowering/errors.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_analysis/src/hir_ty_lowering/errors.rs Outdated Show resolved Hide resolved
tests/ui/associated-types/associated-types-eq-2.stderr Outdated Show resolved Hide resolved
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 29, 2024
@gurry gurry force-pushed the 122162-impl-type-binding-suggestion branch from f31eb72 to fcdffd6 Compare April 1, 2024 03:38
@gurry
Copy link
Contributor Author

gurry commented Apr 1, 2024

@fmease I have updated the code as per your review comments. The main thing to note is that I have changed the approach such that we make the "use directly" suggestion only there is a type param with a matching name. If not, we suggest removal.
@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 1, 2024
@gurry gurry force-pushed the 122162-impl-type-binding-suggestion branch from fcdffd6 to 52e9774 Compare April 3, 2024 10:42
@gurry
Copy link
Contributor Author

gurry commented Apr 3, 2024

☔ The latest upstream changes (presumably #123396) made this pull request unmergeable. Please resolve the merge conflicts.

Rebased

Copy link
Member

@fmease fmease left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for taking so long again! I should have more time for review now.
I have a couple of suggestions.

compiler/rustc_hir_analysis/src/hir_ty_lowering/errors.rs Outdated Show resolved Hide resolved
tests/ui/associated-consts/issue-102335-const.stderr Outdated Show resolved Hide resolved
compiler/rustc_hir_analysis/src/hir_ty_lowering/errors.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_analysis/src/hir_ty_lowering/errors.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_analysis/src/hir_ty_lowering/errors.rs Outdated Show resolved Hide resolved
tests/ui/associated-types/associated-types-eq-2.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_analysis/src/hir_ty_lowering/errors.rs Outdated Show resolved Hide resolved

// Check if the type has a generic param with the
// same name as the assoc type name in type binding
let generics = tcx.generics_of(def_id);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generics_of can panic if the definition (identified by the def_id) can't have generic parameters. Could you check if we can indeed panic here, e.g. if the definition is a type parameter:

fn f<T>() {
    let _: T<X = ()>;
}

If it does, we need to add some checks before trying to use generics_of.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this does not ICE

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 12, 2024
&& let hir::TypeBindingKind::Equality { term } = binding.kind
{
// Suggests removal of the offending binding
let suggest_removal = |e: &mut Diag<'_>| {
Copy link
Member

@fmease fmease Apr 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: I still need to review this function. I suspect it can be simplified by a lot but let's see.

@gurry
Copy link
Contributor Author

gurry commented Apr 13, 2024

Sorry for taking so long again! I should have more time for review now. I have a couple of suggestions.

No worries @fmease.

I'll incorporate your suggestions.

@gurry gurry force-pushed the 122162-impl-type-binding-suggestion branch from 52e9774 to f7ebad4 Compare April 16, 2024 05:42
@gurry
Copy link
Contributor Author

gurry commented Apr 19, 2024

@fmease please review

@gurry
Copy link
Contributor Author

gurry commented Apr 21, 2024

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 21, 2024
@fmease
Copy link
Member

fmease commented Apr 21, 2024

I've left some comments here (linking this from here for visibility).

@fmease fmease added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 21, 2024
@gurry
Copy link
Contributor Author

gurry commented Apr 22, 2024

I've left some comments here (linking this from here for visibility).

Thanks for all the suggestions. They have made the PR so much better.

If you're okay, we can merge this PR and take up the problems around macros in a later PR.

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 22, 2024
@fmease
Copy link
Member

fmease commented Apr 23, 2024

If you're okay, we can merge this PR and take up the problems around macros in a later PR.

👍

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Apr 23, 2024

📌 Commit f7ebad4 has been approved by fmease

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 23, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 23, 2024
Rollup of 7 pull requests

Successful merges:

 - rust-lang#120929 (Wrap dyn type with parentheses in suggestion)
 - rust-lang#122591 (Suggest using type args directly instead of equality constraint)
 - rust-lang#122598 (deref patterns: lower deref patterns to MIR)
 - rust-lang#123048 (alloc::Layout: explicitly document size invariant on the type level)
 - rust-lang#123993 (Do `check_coroutine_obligations` once per typeck root)
 - rust-lang#124218 (Allow nesting subdiagnostics in #[derive(Subdiagnostic)])
 - rust-lang#124285 (Mark ``@RUSTC_BUILTIN`` search path usage as unstable)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 68939f7 into rust-lang:master Apr 23, 2024
12 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Apr 23, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 23, 2024
Rollup merge of rust-lang#122591 - gurry:122162-impl-type-binding-suggestion, r=fmease

Suggest using type args directly instead of equality constraint

When type arguments are written erroneously using an equality constraint we suggest specifying them directly without the equality constraint.

Fixes rust-lang#122162

Changes the diagnostic in the issue from:
```rust
error[E0229]: associated type bindings are not allowed here
9 | impl std::cmp::PartialEq<Rhs = T> for S {
  |                          ^^^^^^^ associated type not allowed here
  |
```
to
```rust
error[E0229]: associated type bindings are not allowed here
9 | impl std::cmp::PartialEq<Rhs = T> for S {
  |                          ^^^^^^^ associated type not allowed here
  |
help: to use `T` as a generic argument specify it directly
  |
  |      impl std::cmp::PartialEq<T> for S {
  |                               ~
```
@gurry gurry deleted the 122162-impl-type-binding-suggestion branch April 24, 2024 00:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
6 participants